Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: Adds mobile menu using AMP #120

Closed
wants to merge 10 commits into from
Closed

WIP: Adds mobile menu using AMP #120

wants to merge 10 commits into from

Conversation

laurelfulford
Copy link
Contributor

@laurelfulford laurelfulford commented Jul 24, 2019 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

This PR is a WIP of adding a mobile-style menu using AMP functionality.

I wanted to share it to get some feedback on the approach -- it does duplicate quite a bit of code, and essentially hides the dupes when not in use (so they're ignored by screen readers, but still there).

AMP does have an approach (toolbar-target) where you can have a nav in the sidebar automatically copied into a specific ID at a certain breakpoint, which would reduce the duplication. However:

  • We'd need to add the duplication back for the non-AMP fallback. So it wouldn't be in the output of an AMP site, but it would be in the PHP files in some form.
  • I'm a little worried the toolbar-target approach would get confusing with the complexity already in the header (it will involve empty divs).
  • Out of the box I got some weird styling trying the toolbar-target approach (a menu displaying in a very small space with scrollbars); but I didn't dig to far into how much work it would be to override.
  • It might also make sense to stick with this duplication approach for now, and optimize it later on.

I'm not against trying the toolbar-target approach, but I didn't want to get too far into the weeds before getting a second opinion.

See #96.

How to test the changes in this Pull Request:

  1. Apply the PR and run npm run build.
  2. Add all the menus; primary, secondary, tertiary.
  3. Make sure AMP is set to 'standard'.
  4. Shrink down the browser window until the mobile styles kick in (the regular menus will disappear and a 'Menu' toggle will display).
  5. Open close the menu; review the appearance.
  6. Check over the code (the bulk of the duplication is in the new mobile-sidebar.php file) and weigh in.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?

@laurelfulford laurelfulford mentioned this pull request Jul 24, 2019
Closed
13 tasks
@laurelfulford laurelfulford force-pushed the adds/amp-mobile-menu branch from 53df7f6 to e66f0a2 Compare July 25, 2019 23:53
@laurelfulford laurelfulford mentioned this pull request Jul 26, 2019
Closed
6 tasks
@laurelfulford
Copy link
Contributor Author

I created a second example PR in #129 that shows the toolbar-target approach -- the menu code only appears once (in the mobile-sidebar.php file, but for a non-AMP fallback it will need to be repeated.

westonruter
westonruter reviewed Jul 31, 2019
View reviewed changes
inc/template-functions.php

if ( 'primary-menu' === $args->theme_location ) :

$nav_menu .= '<div class="main-menu-more">';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This div is causing a validation error here. The AMP plugin is reporting the element as being invalid here because it is violating this constraint in the amp-sidebar > nav tag spec:

  child_tags: {
    mandatory_num_child_tags: 1
    child_tag_name_oneof: "OL"
    child_tag_name_oneof: "UL"
  }

Nevertheless, even though the AMP plugin is reporting it as a validation error, it is not removing it. Also, the markup is not causing validation errors in the AMP Validator. So I'm confused.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what you have is correct here, and there is a bug with the AMP plugin. Namely, because the parent nav does not have a toolbar attribute, this constraint should not be applying.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened a fix: ampproject/amp-wp#2926

@laurelfulford
Copy link
Contributor Author

Thanks for catching and reporting that, @westonruter!

A cleaned up version of the AMP menu was added in #158, so I'm going to close this PR.

@laurelfulford laurelfulford deleted the adds/amp-mobile-menu branch August 9, 2019 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter left review comments

Assignees
No one assigned
Labels
[Status] Needs Discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@laurelfulford @westonruter